-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use newer vega versions for altair compatibility #959
Use newer vega versions for altair compatibility #959
Conversation
Newer versions of vega, vega-lite and vega-embed are used. Additionally, the JavaScript for embedding is adapted. jsdelivr is used instead of cloudflare, since it allows to specify the newest minor version very nicely and it is recommended by the vega-lite docs https://vega.github.io/vega-lite/usage/embed.html#cdn
Hi folium team, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your PR, it is good to keep up to date with these libraries. But since the change from Vega 2 to 3 (and Vega Lite 1 to 2 also?) is not backwards compatible, I would like to give users the option to keep using the previous version. What do you think would be the best way to implement this?
@@ -232,7 +232,7 @@ class VegaLite(Element): | |||
|
|||
def __init__(self, data, width=None, height=None, | |||
left='0%', top='0%', position='relative'): | |||
super(VegaLite, self).__init__() | |||
super(self.__class__, self).__init__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this PR clean and don't change this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow got a strange error at some point, when I used VegaLite
instead of self.__class__
, but I can revert that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the error?
folium/features.py
Outdated
name='d3') | ||
|
||
figure.header.add_child( | ||
JavascriptLink('https://cdnjs.cloudflare.com/ajax/libs/vega/2.6.5/vega.min.js'), # noqa | ||
JavascriptLink('https://cdn.jsdelivr.net/npm/vega@3'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this kind of cdn url
folium/features.py
Outdated
{{this.get_name()}}, embedSpec, function(error, result) {} | ||
); | ||
const spec = {{this.json}}; | ||
vegaEmbed({{this.get_name()}}, spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems vegaEmbed
is the function to use in vega embed version 3, while vg.embed
was used in version 2, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that seems to be the case.
@@ -232,7 +232,7 @@ class VegaLite(Element): | |||
|
|||
def __init__(self, data, width=None, height=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're only updating VegaLite
it seems, why not Vega
also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will do
Thanks for the review! I also thought about leaving support for VegaLite 1, but when I tried it out, it seemed to be backwards compatible at least for a simple examples and I first wanted to find out if this feature is desired at all. I think the easiest way to keep backwards compatible support would be to query the |
Sounds like a good idea. If someone specifies the $schema field, use that version, else use the most recent version. We do have to mention that in the docs though. I was thinking about the best place to do the switching. Maybe in the
Thanks! Looking forward to improving our Vega (Lite) support. |
I finally found time to do this. Let me know what you think of the code, I will add a section describing the changes to the docs then. I now implemented this only for vegalite, as this is relevant for altair. I would prefer to take care of vega in a separate PR. |
@@ -112,7 +113,8 @@ def render(self, **kwargs): | |||
'if it is not in a Figure.') | |||
|
|||
figure.header.add_child( | |||
JavascriptLink('https://cdnjs.cloudflare.com/ajax/libs/leaflet-dvf/0.3.0/leaflet-dvf.markers.min.js'), # noqa | |||
JavascriptLink('https://cdnjs.cloudflare.com/ajax/libs/leaflet-dvf/0.3.0/leaflet-dvf.markers.min.js'), | |||
# noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this all in one line.
folium/features.py
Outdated
@@ -208,6 +210,10 @@ def render(self, **kwargs): | |||
name='vega_parse') | |||
|
|||
|
|||
def get_vega_versions(spec): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used somewhere?
…ium into feature/vega-version-update
Thanks for the review! I incorporated the comments. Regarding the background issue, I will probably wait until #1040 is merged. Then it might work as intended. |
Yep. That is not caused by this PR. |
folium/features.py
Outdated
def _get_vegalite_major_versions(self, spec): | ||
try: | ||
schema = spec['$schema'] | ||
version = os.path.splitext(os.path.split(schema)[1])[0].lstrip('v') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need os
to split schema
here? I'm just thinking that we can avoid one extra import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one could solve it just with str.split
, but the os
functions more direct and have somewhat speaking names. I also think importing os
does not harm as it is in the standard library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +0.5 on that. Indeed os
is standard lib, but it is an extra import that is used for a simple task. I would also argue that .split('/') is more readble than .path.splitext
which is intended for file system paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convinced! Even needs one method call less this way.
except KeyError: | ||
major_version = None | ||
else: | ||
major_version = schema.split('/')[-1].split('.')[0].lstrip('v') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-line and less code 😄
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
folium/features.py
Outdated
|
||
def _vega_embed(self): | ||
self._parent.script.add_child(Element(Template(""" | ||
const spec = {{this.json}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec
here will need a unique name or we won't be able to display more than 1 object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a simple fix by just avoiding using the spec
variable. However, we should test against this case for the future. Unfortunately I have no idea how to do this, as the error is happening on the JS side. Just executing a test with two VegaLite elements does not throw an error.
Putting an example in the example notebooks might be a way, but could look a bit awkward and is also not caught by pytest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a simple fix by just avoiding using the
spec
variable.
Great!
However, we should test against this case for the future. Unfortunately I have no idea how to do this, as the error is happening on the JS side. Just executing a test with two VegaLite elements does not throw an error.
Putting an example in the example notebooks might be a way, but could look a bit awkward and is also not caught by pytest.
Yeah, that is hard. We can create a map and compare an expected output but that may not catch errors like that. We do build all the maps in the example gallery and check them always (sometimes) before merging a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe one could start a headless selenium browser and check if the map and the charts are there. But that would be maybe a bit two much for this PR. I can easily provide a notebook example for now and try to tackle more elaborate testing in a separate PR if that is fine for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That kind of testing is in our TODO list for a long time and I agree, it is something for another PR.
This one is good to go, just waiting for @Conengmo last 👍 to merge it.
Newer versions of vega, vega-lite and vega-embed are used to allow the
usage of altair 2.x. Additionally,
the JavaScript for embedding is adapted. jsdelivr is used instead of
cloudflare, since it allows to specify the newest minor version very
nicely and it is recommended by the vega-lite docs
https://vega.github.io/vega-lite/usage/embed.html#cdn